Implement water_heater platform - part 2#1092
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces legacy max-temperature number entities and the single water-heater entity with separate boiler_temperature and dhw_temperature variants, updates translations, fixtures, snapshots, and tests for the new schema, and switches the Plugwise dependency to the water_heater_2 git target. ChangesWater Heater Multi-Entity Refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@custom_components/plugwise/manifest.json`:
- Line 11: The plugwise dependency URL in the manifest.json contains an extra
forward slash in the git reference syntax. In the string starting with
"plugwise@git+https://github.com/plugwise/python-plugwise.git/@water_heater_2",
remove the forward slash that appears between ".git" and "`@water_heater_2`" so
the URL follows the correct PEP 508 VCS format:
git+https://github.com/plugwise/python-plugwise.git@water_heater_2.
- Around line 10-12: The git requirement URL for the plugwise package in the
requirements array contains a malformed reference syntax with an extraneous
forward slash. In the plugwise requirement string that starts with
"plugwise@git+https://github.com/plugwise/python-plugwise.git/", remove the
forward slash immediately before the @ symbol so that it reads
"plugwise@git+https://github.com/plugwise/python-plugwise.git@water_heater_2"
instead. The @ symbol must attach directly to .git without an intervening slash
for pip to correctly resolve the git branch reference.
In `@custom_components/plugwise/number.py`:
- Around line 110-120: The issue is that self.ctrl is cached once during
initialization, causing the native_value property to return stale data after
coordinator updates. Instead of caching self.ctrl as an instance variable in the
initialization block, remove that line and modify the native_value property to
read directly from the current device state each time it is called by accessing
self.device.get(description.key, {}).get("setpoint"). Keep the initialization of
_attr_native_max_value, _attr_native_min_value, and _attr_native_step since they
define static bounds, but ensure native_value always fetches the fresh setpoint
from the device payload.
In `@custom_components/plugwise/translations/nl.json`:
- Line 307: The "off" label in the Dutch translation file (nl.json) currently
has the English value "Off" instead of the proper Dutch translation. Replace the
value "Off" with the Dutch translation "Uit" for the "off" key to maintain
consistency with other Dutch state labels in the file.
In `@custom_components/plugwise/water_heater.py`:
- Around line 109-110: The OPERATION_MODE feature is being advertised for all
water heater entities regardless of whether they actually have mode options
available. For entities like boiler_temperature where options_key is None, the
OPERATION_MODE feature should not be added to _attr_supported_features. Gate the
addition of WaterHeaterEntityFeature.OPERATION_MODE by checking if the entity
has valid mode options (i.e., options_key is not None) before setting it in the
feature flags. Apply this conditional gating consistently across all water
heater entity classes to prevent invalid mode-control paths.
- Around line 105-112: Remove the caching of `self.dhw_temp` at initialization
and instead use `description.key` dynamically to access per-entity temperature
data. Replace the line that sets `self.dhw_temp = self.device.get(DHW_TEMP, {})`
with direct references to `self.device.get(description.key, {})` when reading
UPPER_BOUND and LOWER_BOUND temperature limits. Apply this same pattern change
to the other affected areas (lines 122-123, 138-140, and 152) to ensure each
entity reads its own temperature configuration key dynamically rather than
caching stale data from initialization.
In `@tests/components/plugwise/snapshots/test_water_heater.ambr`:
- Around line 8-13: The boiler water-heater snapshots in the test file are
validating DHW-derived limits and modes instead of boiler-derived values, and
they contain an inconsistent mode contract where operation_list only contains
"on" while operation_mode or state is set to "eco". Update all affected snapshot
sections (including the ones at the specified line ranges) to use boiler-derived
temperature limits and operation modes, and ensure the operation_list values are
consistent with the actual operation_mode and state values being tested so that
the snapshots accurately validate the boiler entity behavior and catch any
mapping regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 17c1491e-eff1-49b3-8758-2e4ea2f4c9c5
📒 Files selected for processing (23)
custom_components/plugwise/const.pycustom_components/plugwise/manifest.jsoncustom_components/plugwise/number.pycustom_components/plugwise/translations/en.jsoncustom_components/plugwise/translations/nl.jsoncustom_components/plugwise/water_heater.pytests/components/plugwise/fixtures/adam_plus_anna_new/data.jsontests/components/plugwise/fixtures/anna_heatpump_heating/data.jsontests/components/plugwise/fixtures/anna_p1/data.jsontests/components/plugwise/fixtures/anna_v4/data.jsontests/components/plugwise/fixtures/anna_v4_dhw/data.jsontests/components/plugwise/fixtures/m_adam_cooling/data.jsontests/components/plugwise/fixtures/m_adam_heating/data.jsontests/components/plugwise/fixtures/m_adam_heating_off_schedule/data.jsontests/components/plugwise/fixtures/m_adam_jip/data.jsontests/components/plugwise/fixtures/m_anna_heatpump_cooling/data.jsontests/components/plugwise/snapshots/test_number.ambrtests/components/plugwise/snapshots/test_select.ambrtests/components/plugwise/snapshots/test_sensor.ambrtests/components/plugwise/snapshots/test_water_heater.ambrtests/components/plugwise/test_init.pytests/components/plugwise/test_number.pytests/components/plugwise/test_water_heater.py
💤 Files with no reviewable changes (3)
- tests/components/plugwise/test_number.py
- tests/components/plugwise/snapshots/test_number.ambr
- tests/components/plugwise/snapshots/test_sensor.ambr
There was a problem hiding this comment.
🧹 Nitpick comments (1)
custom_components/plugwise/translations/nl.json (1)
100-106: 📐 Maintainability & Code Quality | 🔵 TrivialVerify completeness of
select_dhw_modetranslations.The translation files for
select_dhw_modecurrently define onlycomfortandeco. As the available options are populated dynamically from the device, ensure that all other potential DHW modes supported by the firmware (e.g.,anti_freeze,boost,off) are included in the translation keys to prevent untranslated strings in the UI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@custom_components/plugwise/translations/nl.json` around lines 100 - 106, The select_dhw_mode translation is incomplete: only comfort and eco are defined in the select state map. Update the translation entries for select_dhw_mode in the nl.json select section to include every DHW mode the device can expose, such as anti_freeze, boost, and off, so the UI has localized labels for all dynamically populated options.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@custom_components/plugwise/translations/nl.json`:
- Around line 100-106: The select_dhw_mode translation is incomplete: only
comfort and eco are defined in the select state map. Update the translation
entries for select_dhw_mode in the nl.json select section to include every DHW
mode the device can expose, such as anti_freeze, boost, and off, so the UI has
localized labels for all dynamically populated options.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 34a1eae3-127e-4e4f-bbe9-c98d1edf4867
📒 Files selected for processing (5)
custom_components/plugwise/manifest.jsoncustom_components/plugwise/number.pycustom_components/plugwise/translations/en.jsoncustom_components/plugwise/translations/nl.jsoncustom_components/plugwise/water_heater.py
🚧 Files skipped from review as they are similar to previous changes (3)
- custom_components/plugwise/number.py
- custom_components/plugwise/manifest.json
- custom_components/plugwise/translations/en.json
7e333a8 to
cd937d8
Compare
e52e3b0 to
370d28b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
custom_components/plugwise/water_heater.py (2)
45-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove leftover dev-note comment before merge.
Line 45 reads:
# Upstream + is there a reason we didn't rename this one prefixed?— this is an open question left in code, not a real code comment. Resolve the naming question or drop the comment before merging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@custom_components/plugwise/water_heater.py` around lines 45 - 59, Remove the leftover developer note from the WATERHEATER_TYPES definition in Plugwise water_heater.py; the inline “Upstream + is there a reason we didn't rename this one prefixed?” text is an unresolved review question, so either rename the related BOILER_TEMP/DHW_TEMP identifiers consistently if that was the intended change or delete the comment entirely and keep only the entity description declarations.
116-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify redundant on/off state staging.
Tracing all branches of
async_set_operation_mode: whether it goes throughasync_turn_off,async_turn_on, or the directelse, it always ends up callingset_dhw_mode(DHW_MODE, self._dev_id, list_type, self._operation_mode)with the same final value. Theself._mode_offcache (set once at init from a snapshot ofDHW_MODE, later mutated only inside these two methods) doesn't change the actual command sent to the device — it only adds branching and shared mutable instance state (_mode_off,_operation_mode,_list_type) that can drift from live coordinator data between calls. This is dead complexity rather than a functional bug today, but it's a maintenance/readability trap for future changes.♻️ Proposed simplification
self._attr_temperature_unit = UnitOfTemperature.CELSIUS self._attr_unique_id = f"{device_id}-{description.key}" - self._list_type = 0 - self._mode_off = self.device.get(DHW_MODE) == STATE_OFF - self._operation_mode: str = STATE_OFFasync def async_set_operation_mode(self, operation_mode: str) -> None: """Set the operation mode.""" if self.operation_list is None: return # pragma: no cover - self._list_type = len(self.operation_list) - self._operation_mode = operation_mode - if self._operation_mode == STATE_OFF and not self._mode_off: - await self.async_turn_off() - return - - if self._mode_off and self._operation_mode != STATE_OFF: - await self.async_turn_on() - return - - await self.coordinator.api.set_dhw_mode(DHW_MODE, self._dev_id, self._list_type, self._operation_mode) + list_type = len(self.operation_list) + await self.coordinator.api.set_dhw_mode( + DHW_MODE, self._dev_id, list_type, operation_mode + )If
async_turn_on/async_turn_offare kept for a futureON_OFFfeature, they can remain as thin wrappers without the_mode_offbookkeeping.Also applies to: 149-195
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@custom_components/plugwise/water_heater.py` around lines 116 - 118, Simplify the redundant operation-mode staging in WaterHeaterEntity by removing the shared mutable cache used for on/off branching. In async_set_operation_mode, async_turn_on, and async_turn_off, stop relying on _mode_off, _operation_mode, and _list_type to decide the final command, and instead derive the DHW mode directly from the requested operation mode before calling set_dhw_mode. Keep async_turn_on and async_turn_off only as thin wrappers if needed, but eliminate the extra state bookkeeping so the behavior is driven by the current request rather than stale instance fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@custom_components/plugwise/water_heater.py`:
- Around line 45-59: Remove the leftover developer note from the
WATERHEATER_TYPES definition in Plugwise water_heater.py; the inline “Upstream +
is there a reason we didn't rename this one prefixed?” text is an unresolved
review question, so either rename the related BOILER_TEMP/DHW_TEMP identifiers
consistently if that was the intended change or delete the comment entirely and
keep only the entity description declarations.
- Around line 116-118: Simplify the redundant operation-mode staging in
WaterHeaterEntity by removing the shared mutable cache used for on/off
branching. In async_set_operation_mode, async_turn_on, and async_turn_off, stop
relying on _mode_off, _operation_mode, and _list_type to decide the final
command, and instead derive the DHW mode directly from the requested operation
mode before calling set_dhw_mode. Keep async_turn_on and async_turn_off only as
thin wrappers if needed, but eliminate the extra state bookkeeping so the
behavior is driven by the current request rather than stale instance fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 889d7da9-2972-42d3-99a2-f63ba6637a1b
📒 Files selected for processing (24)
custom_components/plugwise/const.pycustom_components/plugwise/manifest.jsoncustom_components/plugwise/number.pycustom_components/plugwise/translations/en.jsoncustom_components/plugwise/translations/nl.jsoncustom_components/plugwise/water_heater.pytests/components/plugwise/fixtures/adam_plus_anna_new/data.jsontests/components/plugwise/fixtures/anna_heatpump_heating/data.jsontests/components/plugwise/fixtures/anna_loria_cooling_active/data.jsontests/components/plugwise/fixtures/anna_p1/data.jsontests/components/plugwise/fixtures/anna_v4/data.jsontests/components/plugwise/fixtures/anna_v4_dhw/data.jsontests/components/plugwise/fixtures/m_adam_cooling/data.jsontests/components/plugwise/fixtures/m_adam_heating/data.jsontests/components/plugwise/fixtures/m_adam_heating_off_schedule/data.jsontests/components/plugwise/fixtures/m_adam_jip/data.jsontests/components/plugwise/fixtures/m_anna_heatpump_cooling/data.jsontests/components/plugwise/snapshots/test_number.ambrtests/components/plugwise/snapshots/test_select.ambrtests/components/plugwise/snapshots/test_sensor.ambrtests/components/plugwise/snapshots/test_water_heater.ambrtests/components/plugwise/test_init.pytests/components/plugwise/test_number.pytests/components/plugwise/test_water_heater.py
💤 Files with no reviewable changes (4)
- tests/components/plugwise/snapshots/test_number.ambr
- custom_components/plugwise/number.py
- tests/components/plugwise/test_number.py
- tests/components/plugwise/snapshots/test_sensor.ambr
✅ Files skipped from review due to trivial changes (5)
- tests/components/plugwise/fixtures/anna_p1/data.json
- tests/components/plugwise/fixtures/m_adam_heating_off_schedule/data.json
- tests/components/plugwise/snapshots/test_select.ambr
- tests/components/plugwise/fixtures/anna_v4_dhw/data.json
- tests/components/plugwise/fixtures/m_adam_cooling/data.json
🚧 Files skipped from review as they are similar to previous changes (11)
- custom_components/plugwise/manifest.json
- tests/components/plugwise/fixtures/adam_plus_anna_new/data.json
- tests/components/plugwise/test_init.py
- tests/components/plugwise/fixtures/anna_heatpump_heating/data.json
- custom_components/plugwise/translations/nl.json
- tests/components/plugwise/fixtures/m_adam_jip/data.json
- tests/components/plugwise/fixtures/m_adam_heating/data.json
- custom_components/plugwise/translations/en.json
- tests/components/plugwise/fixtures/m_anna_heatpump_cooling/data.json
- tests/components/plugwise/fixtures/anna_v4/data.json
- custom_components/plugwise/const.py
|
| self._attr_max_temp = temp_data.get(UPPER_BOUND, 75.0) | ||
| self._attr_min_temp = temp_data.get(LOWER_BOUND, 40.0) |
There was a problem hiding this comment.
While we are at it, lets set these in const as well?
|
Just marking approved for not reading through all the jsons again :) Nothing problematic though aside from a single nitpick |



Summary by CodeRabbit
New Features
Changes
Chores